Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

std: expose ChildProcess's exec function #10436

Closed
wants to merge 3 commits into from
Closed

std: expose ChildProcess's exec function #10436

wants to merge 3 commits into from

Conversation

nektro
Copy link
Contributor

@nektro nektro commented Dec 29, 2021 •
edited
Loading

found this as a cleaner solution than exposing collectOutputPosix and collectOutputWindows separately.

spawnAndWait alone does not solve this use case since it does not collect any output

what this does instead is rename exec to initAndExec and makes exec a standalone method to get the term and output

edit: feel free to squash commit this

@daurnimator
Copy link
Contributor

If we're renaming this method, can we please move away from "exec" as a name? It suggests that the current process will be replaced.

See #5853

@daurnimator
Copy link
Contributor

what name should be used instead?

I think spawn seems the best suggestion in there; and it has precedence via e.g. posix_spawn in C.

@nektro
Copy link
Contributor Author

nektro commented Dec 30, 2021

spawn is already a method and does something different

@andrewrk andrewrk added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label May 11, 2022
andrewrk
andrewrk requested changes May 11, 2022
View reviewed changes
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @daurnimator - this is a breaking change to rename a function which has an accepted proposal to avoid "exec" as the name, so the breaking change should follow this guidance.

Also, this is a breaking change that I'm not sure what the motivation is. Can I see an example of what didn't work very well without this breaking change?

@andrewrk andrewrk added the standard library This issue involves writing Zig code for the standard library. label May 11, 2022
@andrewrk andrewrk added this to the 0.11.0 milestone May 11, 2022
@andrewrk andrewrk added the enhancement Solving this issue will likely involve adding new logic or components to the codebase. label May 11, 2022
@andrewrk andrewrk removed this from the 0.11.0 milestone May 26, 2022
@andrewrk andrewrk closed this May 26, 2022
@nektro nektro deleted the childprocess-exec branch May 27, 2022 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewrk andrewrk andrewrk requested changes

Assignees
No one assigned
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nektro @daurnimator @andrewrk